-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
smb: add smb.version keyword 5075 v4 #9469
Conversation
Signed-off-by: jason taylor <[email protected]>
Signed-off-by: jason taylor <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9469 +/- ##
==========================================
- Coverage 82.18% 82.17% -0.02%
==========================================
Files 968 969 +1
Lines 274199 274283 +84
==========================================
+ Hits 225363 225381 +18
- Misses 48836 48902 +66
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation change looks good, commit separation and authoring, too.
Left a common wrt the copyright years for the files to be added, not sure about how to proceed there.
@@ -0,0 +1,159 @@ | |||
/* Copyright (C) 2022-2023 Open Information Security Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very sorry for not noticing this before, but technically speaking, since we're only adding this now, this should be 2023 only, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the original commits/work from Eloy being from last year, I just added this year. Of course willing to update with whatever you all see fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. I think we use the year of when the code is merged, but not sure in such cases, so can't decide...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work
- CI : ✅
- Code : good enough for me
- Commits segmentation : ok (I would have squashed the 2 commits from Eloy)
- Commit messages : Fair enough for me
- Git ID set : looks fine for me
- CLA : I do not have access, but looks like some commits were already merged
- Doc update : really nice 👏
- Redmine ticket : ok
- Rustfmt : my rustfmt wants a new line at the end of rust/src/smb/detect.rs
- Tests : Suricata-verify tests look fine
- Dependencies added: none
Seeing as how #9852 was merged. I was wondering if you all would like me to submit an updated PR with the appropriate changes and of course rebased? |
Thanks for catching that up, if you could, that'd be great! :) :) |
Yes @jmtaylor90 please rebase this :-) |
continued in #9905 |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5075
Describe changes:
Provide values to any of the below to override the defaults.
To use a pull request use a branch name like
pr/N
whereN
is thepull request number.
Alternatively,
SV_BRANCH
may also be a link to anOISF/suricata-verify pull-request.